Limit the size of served files #834
Conversation
This will allow tests to ensure data with any compression algorithm ever used works, instead of just testing if the current algorithm works.
| let alg = DEFAULT_COMPRESSION; | ||
| let content = compress(file, alg)?; |
There was a problem hiding this comment.
What's the purpose of setting a variable to DEFAULT_COMPRESSION instead of using it directly?
There was a problem hiding this comment.
I put DEFAULT_COMPRESSION in a variable as that makes changing the function to accept the algorithm as a parameter easier in the future. I'm not too attached to that though, I can surely change this.
|
LGTM |
|
Nevermind, also added the final test. |
| } | ||
|
|
||
| impl Config { | ||
| pub fn from_env() -> Result<Self, Error> { |
There was a problem hiding this comment.
The idea is that over time we'll move more of the environment variables here, instead of being spread out over the codebase? I like it, it makes things much easier to test as well.
There was a problem hiding this comment.
Yep, that's the goal!
There was a problem hiding this comment.
I completely support that, having a concrete storage of env vars in one place would be fantastic
|
Looks good to me modulo nits. |
|
@jyn514 addressed the nits. I also updated how tests can override configuration values to this: env.override_config(|config| {
config.key = value;
});Using the closure allows to change the default configuration for tests in the future without having to update the base config in each test that overrides it. |
|
Looks great, thanks! |
This PR limits how big the files we serve from docs.rs are, to avoid OOMs. The current limit is 5MB, but that can be changed by tweaking a constant in the source code.
This PR adds limits when downloding the files from S3 and the database, and when decompressing previously downloaded files. For now the user will see a "resource not found" error page, but a dedicated page can be created in the future.
This PR also refactors compression a bit to better enable testing. It's best to review the PR commit-by-commit.
r? @jyn514